Docs Build 📝

Thank you for contribution!✨

The docsite for this PR is available for download as an artifact from this run:
https://github.com/ansible-collections/microsoft.ad/actions/runs/7369963794

You can compare to the docs for the main branch here:
https://ansible-collections.github.io/microsoft.ad/branch/main

File changes:

• A collections/microsoft/ad/acl_module.html
• M collections/index_module.html
• M collections/microsoft/ad/computer_module.html
• M collections/microsoft/ad/docsite/guide_migration.html
• M collections/microsoft/ad/index.html

diff --git a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/index_module.html b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/index_module.html
index 905b056..49bd556 100644
--- a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/index_module.html
+++ b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/index_module.html
@@ -124,6 +124,7 @@
 <section id="microsoft-ad">
 <h2>microsoft.ad<a class="headerlink" href="#microsoft-ad" title="Link to this heading"></a></h2>
 <ul class="simple">
+<li><p><a class="reference internal" href="microsoft/ad/acl_module.html#ansible-collections-microsoft-ad-acl-module"><span class="std std-ref">microsoft.ad.acl</span></a> – Used to set ACL’s on objects in an Active Directory.</p></li>
 <li><p><a class="reference internal" href="microsoft/ad/computer_module.html#ansible-collections-microsoft-ad-computer-module"><span class="std std-ref">microsoft.ad.computer</span></a> – Manage Active Directory computer objects</p></li>
 <li><p><a class="reference internal" href="microsoft/ad/debug_ldap_client_module.html#ansible-collections-microsoft-ad-debug-ldap-client-module"><span class="std std-ref">microsoft.ad.debug_ldap_client</span></a> – Get host information for debugging LDAP connections</p></li>
 <li><p><a class="reference internal" href="microsoft/ad/domain_module.html#ansible-collections-microsoft-ad-domain-module"><span class="std std-ref">microsoft.ad.domain</span></a> – Ensures the existence of a Windows domain</p></li>
diff --git a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/computer_module.html b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/computer_module.html
index 4fd060a..decf642 100644
--- a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/computer_module.html
+++ b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/computer_module.html
@@ -25,7 +25,7 @@
     <script src="../../../_static/js/theme.js"></script>
     <link rel="search" title="Search" href="../../../search.html" />
     <link rel="next" title="microsoft.ad.debug_ldap_client module – Get host information for debugging LDAP connections" href="debug_ldap_client_module.html" />
-    <link rel="prev" title="Migration guide" href="docsite/guide_migration.html" /><!-- extra head elements for Ansible beyond RTD Sphinx Theme -->
+    <link rel="prev" title="microsoft.ad.acl module – Used to set ACL’s on objects in an Active Directory." href="acl_module.html" /><!-- extra head elements for Ansible beyond RTD Sphinx Theme -->
 
 
 
@@ -724,7 +724,7 @@ see <a class="reference internal" href="#ansible-collections-microsoft-ad-comput
           
 
 <footer><div class="rst-footer-buttons" role="navigation" aria-label="Footer">
-        <a href="docsite/guide_migration.html" class="btn btn-neutral float-left" title="Migration guide" accesskey="p" rel="prev"><span class="fa fa-arrow-circle-left" aria-hidden="true"></span> Previous</a>
+        <a href="acl_module.html" class="btn btn-neutral float-left" title="microsoft.ad.acl module – Used to set ACL’s on objects in an Active Directory." accesskey="p" rel="prev"><span class="fa fa-arrow-circle-left" aria-hidden="true"></span> Previous</a>
         <a href="debug_ldap_client_module.html" class="btn btn-neutral float-right" title="microsoft.ad.debug_ldap_client module – Get host information for debugging LDAP connections" accesskey="n" rel="next">Next <span class="fa fa-arrow-circle-right" aria-hidden="true"></span></a>
     </div>
 
diff --git a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/docsite/guide_migration.html b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/docsite/guide_migration.html
index f420d7d..520aceb 100644
--- a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/docsite/guide_migration.html
+++ b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/docsite/guide_migration.html
@@ -23,7 +23,7 @@
         <script src="../../../../_static/sphinx_highlight.js?v=dc90522c"></script>
     <script src="../../../../_static/js/theme.js"></script>
     <link rel="search" title="Search" href="../../../../search.html" />
-    <link rel="next" title="microsoft.ad.computer module – Manage Active Directory computer objects" href="../computer_module.html" />
+    <link rel="next" title="microsoft.ad.acl module – Used to set ACL’s on objects in an Active Directory." href="../acl_module.html" />
     <link rel="prev" title="Setting list option values guide" href="guide_list_values.html" /><!-- extra head elements for Ansible beyond RTD Sphinx Theme -->
 
 
@@ -281,7 +281,7 @@
 
 <footer><div class="rst-footer-buttons" role="navigation" aria-label="Footer">
         <a href="guide_list_values.html" class="btn btn-neutral float-left" title="Setting list option values guide" accesskey="p" rel="prev"><span class="fa fa-arrow-circle-left" aria-hidden="true"></span> Previous</a>
-        <a href="../computer_module.html" class="btn btn-neutral float-right" title="microsoft.ad.computer module – Manage Active Directory computer objects" accesskey="n" rel="next">Next <span class="fa fa-arrow-circle-right" aria-hidden="true"></span></a>
+        <a href="../acl_module.html" class="btn btn-neutral float-right" title="microsoft.ad.acl module – Used to set ACL’s on objects in an Active Directory." accesskey="n" rel="next">Next <span class="fa fa-arrow-circle-right" aria-hidden="true"></span></a>
     </div>
 
   <hr/>
diff --git a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/index.html b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/index.html
index d00f9e4..b486036 100644
--- a/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/base/collections/microsoft/ad/index.html
+++ b/home/runner/work/microsoft.ad/microsoft.ad/docsbuild/head/collections/microsoft/ad/index.html
@@ -189,6 +189,7 @@
 <section id="modules">
 <h3>Modules<a class="headerlink" href="#modules" title="Link to this heading"></a></h3>
 <ul class="simple">
+<li><p><a class="reference internal" href="acl_module.html#ansible-collections-microsoft-ad-acl-module"><span class="std std-ref">acl module</span></a> – Used to set ACL’s on objects in an Active Directory.</p></li>
 <li><p><a class="reference internal" href="computer_module.html#ansible-collections-microsoft-ad-computer-module"><span class="std std-ref">computer module</span></a> – Manage Active Directory computer objects</p></li>
 <li><p><a class="reference internal" href="debug_ldap_client_module.html#ansible-collections-microsoft-ad-debug-ldap-client-module"><span class="std std-ref">debug_ldap_client module</span></a> – Get host information for debugging LDAP connections</p></li>
 <li><p><a class="reference internal" href="domain_module.html#ansible-collections-microsoft-ad-domain-module"><span class="std std-ref">domain module</span></a> – Ensures the existence of a Windows domain</p></li>

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/f4b16d2f415d4b83bebf6a3f722f7b36

✔️ ansible-galaxy-importer SUCCESS in 4m 47s
✔️ build-ansible-collection SUCCESS in 9m 07s

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/3eeacc28c0b24110a7c65214bde073c8

✔️ ansible-galaxy-importer SUCCESS in 5m 00s
✔️ build-ansible-collection SUCCESS in 6m 56s

Thanks for the PR, I'll be traveling next week so won't have too much time to look into this. One thing I would mention is that this might be a good option for setting single rules but we might need to look into making things more efficient by allowing multiple rules to be set. The UX behind such a module is going to be difficult to work out here unfortunately.

Hi,

I also considered making both rights and rights_attr arrays and then generate rules for all permutations, another alternative is adding a list with rights/rights_attr and adding all these. One thing to consider is that rights_attr is optional, just giving a rights will be set on all attributes unless rights_attr is specified.

/Mikael

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/665def086ad74f10aaeeb85136cc47da

✔️ ansible-galaxy-importer SUCCESS in 9m 36s
✔️ build-ansible-collection SUCCESS in 6m 47s

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/4198b86cd526494d867455818b0b5f97

✔️ ansible-galaxy-importer SUCCESS in 4m 38s
✔️ build-ansible-collection SUCCESS in 6m 43s

Hi,

I did some minor cleanups, the rights argument is handled by the enum-type so it can already by multiple values. I also added the inherited type so it is possible to let a user/group get access to children of a specific type, see the examples in the python-file

/Mikael

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/95326a483fe6410b9ee48f2054aa2cbf

✔️ ansible-galaxy-importer SUCCESS in 4m 21s
✔️ build-ansible-collection SUCCESS in 7m 31s

Sorry for not getting back to you on this beforehand, I've been tied up in some unrelated work to this collection and have unfortunately left this on the backlog. The changes you have here seem like a decent module but I'm not sure if it's the direction to go in. I don't want to leave it open any longer without me actually replying to the hard work you've put in here and list the reasons why I haven't done a full review merge yet.

Looking at the code here it definitely resembles how the win_acl module works but I've been apprehensive about replicating that module for a few reasons:

• It's inefficient when you need to deal with multiple rules (one module invocation per rule)
• It only covers DACLs and not owner/group/sacl/integrity label
• It works on an exact match only
  • Should it be ignoring or comparing against inherited rules
  • Should it consider a rule with the same principal and type but with an access mask that covers more permissions as present or not?
• How should inheritance be dealt with, there is win_acl_inheritance but this is yet another module invocation

It is also missing things like check mode, diff support, and tests which is something I'm very keen on having on all the modules in this collection. This is not a complaint against the code you've written, just something that would need to be added if we choose to continue down the option you are proposing here.

I keep on coming back to the idea of a security_descriptor module with something like:

- microsoft.ad.security_descriptor:
    # Selects the object to manage
    name: obj
    identity: obj
    path: ...
    # SD bits
    owner: sid or account name
    group: sid or account name
    dacl:
      add:
      - identity: sid or account name
        rights: list of numbers or known string flags
        type: allow|deny
        object_type: guid or name
        inheritance_type: all|children|descendents|none|self_and_children
        inherited_object_type: guid or name
        # Defines how to compare to existing rules, exact is exact while
        # rights must have the rights set in the bitmask
        compare_rule: exact|rights
      remove: ...
      set: ...
      inheritance: absent|present
      # When inheritance: absent, converts to explicit rules, when present
      # dedups explicit rules if they match newly added inherited ones
      inheritance_reorganize: true|false
    sacl:
      add:
      - identity: sid or account name
        rights: list of numbers or known string flags
        type: none|failure|success
        object_type: guid or name
        inheritance_type: all|children|descendents|none|self_and_children
        inherited_object_type: guid or name
        compare_rule: exact|rights
      remove: ...
      set: ...
      inheritance: absent|present
      inheritance_perserve: true|false
    label:
      flags: no_write_up|no_read_up|no_execute_up
      value: untrusted|low|medium|high|system|protected_process

There's also nothing stopping having this as a more efficient module for doing a one big definition vs also making a owner, dacl, and sacl module just for those specific components with the same options. My apprehension about this approach is that this is a fairly complex definition so people looking for an easy way to define just a single rule might be confused as to how it is done.

Ultimately I think I am leaning towards having the following separate modules to match each of the entries in the definition above

• microsoft.ad.sd_owner - Sets the security descriptor owner
• microsoft.ad.sd_group - Sets the security descriptor group (probably don't really need this)
• microsoft.ad.sd_dacl - Sets the security descriptor DACL ACEs (access rights)
• microsoft.ad.sd_sacl - Sets the security descriptor SACL ACEs (audit rules)
• microsoft.ad.sd_label - Sets the security descriptor integrity label
• microsoft.ad.sd_info - Returns the existing SD in a structure similar to the above

I could even go a step further an make those modules more of a module util and still do the big microsoft.ad.sd or microsoft.ad.security_descriptor that still provides a way to specify all these at the same time in one module if desired.

I am curious as to your thoughts on all this and what you think about the proposal. Knowing how someone actually would use this in the real world will definitely help shape how it all works.

I like your idea and I think both the security_descriptor and the separate modules will work, the problem is the rights system is complex, even more so in an Active Directory, and I think it will be hard to "simplyfy" the modules to much, the modules needs to be complex to handle complex rules.

The reason I needed to modify the acl's was to have an OU with a group and a number of normal users and the users should be able to create additional users in the OU and also add them to the group, for this one rule is needed on the OU to allow for new users to be created, one rule for modify users in the OU and one rule for modifying the group. These are the examples in the pull request. For this I think I would prefer the separate modules solution, it would be three microsoft.ad.sd_dacl rules since this is what I would change.

I like that you have the set-option in all modules, I like to set the correct value and then be able to verify that nothing new has been added.

Yeah, I concur with most of @jborean93 's thoughts here- the functionality and UX requirements we're trying to enforce for new modules in "officially supported" collections are a bit more onerous. Speaking from our own experiences trying to do so, it's often difficult to retrofit things like multiple resource support and "add vs replace" to existing modules without a bunch of complex heuristics or confusing the UX with weird mode args. The results are a lot better when those things are designed in from the beginning. The fact that it's somewhat difficult to canonically "key" an ACE and definitively say "yes, this ACE/ACL is logically equivalent to that one" makes it even gnarlier- I remember pulling my own hair out trying to write a basic idempotent ACL manipulation of a random system object for an installer ~ 20 years ago.

I also agree that one invocation per ACE is probably not the right way to go- for DACLs anyway, the resource granularity should be (at minimum) the DACL, not the individual ACEs. I'm a bit torn on the separate modules thing; IME owner and dacl are by far the most commonly used things, so focusing on those is probably most important. Since the underlying resource really is just the SD, and the DACL is probably what most folks are after, I'm not sure it'd be so bad to just have one sd module... Yeah, a task that actually sets everything could be kinda gnarly, but I don't think the added complexity of sharing it all in one module outweighs the overhead of multiple modules (esp since IIRC the idempotence logic for owner/group/dacl/sacl/label wouldn't need to interact with each other). I'm guessing that a typical invocation of an all-singing-all-dancing sd module to tweak a dacl wouldn't look significantly different than if it were a separate dacl module, and I'd imagine it simplifies the SD management somewhat to have one big module.

@jborean93 were your concerns about a single SD module mostly related to UX/docs or implementation?

@jborean93 were your concerns about a single SD module mostly related to UX/docs or implementation?

The latter. Trying to document the somewhat nested structures and the values will get quite complex, especially once we start adding in support for SACLs. At least with smaller modules the documentation would be focused specifically on that one component.

Regardless of whether this was one giant module vs separate ones for each SD component the implementation will most likely have to be the same. The owner/group ones are simple but as you said trying to determine if an ACE is present in an ACL will be where the complexity lies.

Fair enough, and after thinking about it a little more, I suppose since the SD itself isn't named, you're never going to "create one from scratch", you're just adjusting properties on an existing object. I can definitely see the attraction to removing a layer of nesting, since our docs and argspec stuff for complex nested structures has improved, but is still "worse than flat".

That said, I did have another minor twist on the UX you described above that gives you the same level of nesting as discrete modules but still all in one module: top-level attr keys for owner/group/label, but then dacl_add/dacl_set/dacl_inherit/dacl_flatten (and similar for sacl if/when it gets added).

I don't know why the multiple tiny modules thing feels icky for this case, but it kinda does. I think maybe it's the thought of the "creating an entirely new OU/object" case possibly requiring ~5 module invocations instead of 1-2. 🤷

That said, I did have another minor twist on the UX you described above that gives you the same level of nesting as discrete modules but still all in one module: top-level attr keys for owner/group/label, but then dacl_add/dacl_set/dacl_inherit/dacl_flatten (and similar for sacl if/when it gets added).

I'm not sure if that would really help the situation too much. The docs will still be quite large as the entries for add/set/remove would need duplicate entries (or we keep it as "raw" and not have validation). The same values would also probably be the same or at least very similar with the SACL side which also has an add/remove/set.

I don't know why the multiple tiny modules thing feels icky for this case, but it kinda does. I think maybe it's the thought of the "creating an entirely new OU/object" case possibly requiring ~5 module invocations instead of 1-2. 🤷

Yea I personally prefer the idea of just 1 module for this but was ultimately planning on having both from a documentation and ease of discoverability for the user. They can all reference each other and ultimately the code will be using a common module_util so we don't have to worry about code duplication here.

Build succeeded.
https://ansible.softwarefactory-project.io/zuul/buildset/93f052a4b27649e0b5ce8337bcfe43a8

✔️ ansible-galaxy-importer SUCCESS in 4m 38s
✔️ build-ansible-collection SUCCESS in 7m 26s

Personally, I would prefer having one "big" module which can basically do everything that Get-ACL/Set-ACL can do (modify owner/group/dacl/sacl) as well.

IMO, starting with add/remove only handling exact matches would be perfectly fine. Adding options to allow fuzzier matching is still possible later on.
If at all possible (no other roles or stuff outside of ansible modifying the same ACL), I would personally always use set anyway, since with this I can be 100% sure of the outcome.

@jborean93 I am not quite sure about the meaning of inheritance, inheritance_reorganize and interhitance_preserve in your example.
IMO,
a) ignoring inherited DACL/SACL
b) having an option to break inheritance
would be sufficient for most use-cases.

I have one more feature idea that would be really helpful: Having an option to ignore the default objectClass ACE (from defaultsecuritydescriptor) when using set.

In summary, I would very much appreciate a module to manage AD ACLs! :)

I have just opened an issue in the ansible.windows collection to discuss designing a better win_acl module. (The current one lacks any kind of "set" functionality, it can only manage a single ACE at a time)

A feature I mentioned there, which also seems relevant to this AD ACL module, is having some kind of recursive mode, to realiably remove all non-inherited/non-managed/non-default ACEs from child objects.